[wrangler] fix: don't crash wrangler dev when source-mapping a truncated error chunk#14316
Conversation
|
Codeowners approval required for this PR:
Show detailed file reviewers |
- Move for-loop inside try block so sourceMappedStackTrace can be const - Capture caught error and log at debug level instead of silently dropping it
🦋 Changeset detectedLatest commit: eee9897 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
0ee4112 to
e7325f9
Compare
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
The 'Tests (Windows, packages-and-tools)' failure is a pre-existing flake in miniflare's browser tests unrelated to this PR. The Chrome DLL gets locked between parallel test processes on the Windows runner. The same failure appears on main without our changes. |
petebacondarwin
left a comment
There was a problem hiding this comment.
I think it would be helpful to have an automated regression test if possible.
Please can you see if you can create one?
|
Addressed all the review feedback in the latest commit: Narrowed try-catch to only wrap Fixed Added regression test in |
When the cached Chrome binary exists but is corrupt, the retry path calls install() again, which fires the downloadProgressCallback and starts the spinner. Without a matching s.stop(), the spinner runs indefinitely through the subsequent launch() and waitForBrowserReady() calls, leaving the terminal in a broken state. Reset startedDownloading before the retry so the callback re-arms cleanly, then stop the spinner after install() returns (success path) or throws (failure path), mirroring the pattern on lines 163-175. Addresses Devin review comment on PR cloudflare#14316.
|
@matingathani - this seems to have picked up the changes from another PR too? |
Wraps the prepareStack call in getSourceMappedString in a try-catch so that if the source map library throws (e.g. because a truncated stderr chunk produced a call site with an invalid column number), the original un-source-mapped string is returned instead of crashing wrangler dev.
…and fix null check - Narrow try-catch to wrap only `prepareStack()` and early-return on failure, so the for-loop replacements are never partially applied then silently dropped - Fix `getFileName() === undefined` → `=== null` (`CallSite.getFileName()` returns `string | null`, never `undefined`, so the old guard always evaluated false) - Add regression test verifying `getSourceMappedString` returns the original value when source mapping fails instead of throwing
…able let Factors the try-catch into a dedicated `tryPrepareStack` helper that returns `null` on failure instead of throwing, letting `getSourceMappedString` use `const` for the result and check `=== null` before entering the for-loop. Addresses Copilot review feedback (prefer const over let-bridging a try block) and aligns with petebacondarwin's suggestion to make prepareStack non-throwing.
82d628e to
f179b04
Compare
|
Cleaned up — the branch had picked up two miniflare/browser-rendering commits (
The diff is now clean — only |
…eStack The catch block was silently swallowing exceptions, making it hard to diagnose unexpected source-map regressions. Now logs at debug level before returning null so the fallback is still transparent to users but visible with --log-level=debug.
…PrepareStack logger from shared/context is undefined until initDeployHelpersContext() is called. Tests that call getSourceMappedString directly never initialize the context, so logger?.debug avoids a TypeError when the logger has not been set up.
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
|
Windows CI failure — pre-existing miniflare browser rendering flake The The failure is the same Chrome connectivity issue on Windows CI that appears on other PRs too (Chrome process startup timeout / DLL lock on Windows runners). @petebacondarwin already has |
Fixes #9919.
When a worker throws errors in rapid succession, the stderr chunks received by wrangler dev can arrive mid-frame — so a call site ends up with a negative column number. The
@jridgewell/trace-mappinglibrary throwsError: \column` must be greater than or equal to 0` in that case, which was bubbling up uncaught and crashing the wrangler process.The fix wraps the
prepareStack(...)call ingetSourceMappedStringin atry/catch. If source mapping throws for any reason, we fall back to returning the original (un-source-mapped) string. The worker process keeps running and the user still sees the error message, just without source map translation.I reproduced this with the snippet from the issue:
Hitting the worker endpoint a few times caused
wrangler devto crash before this fix. After the fix, it logs the raw stack and keeps running.